Skip to content

Refactor/travel#224

Merged
chungjeongsu merged 8 commits into
developfrom
refactor/travel
Apr 24, 2026
Merged

Refactor/travel#224
chungjeongsu merged 8 commits into
developfrom
refactor/travel

Conversation

@chungjeongsu

Copy link
Copy Markdown
Contributor

No description provided.

@chungjeongsu chungjeongsu self-assigned this Apr 24, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the travel itinerary management system into a UseCase-Command-Reader architecture, removes soft-delete logic, and implements Querydsl for repository queries. Feedback identifies missing validations for member existence and group membership, a date validation bug in entity setters, redundant database fetches, and inconsistent error handling.

Comment on lines +44 to +46
List<Long> memberIds = req.memberUuids().stream().map(userIdentityResolver::resolve).toList();
List<User> members = userReader.findUsersInIds(memberIds).stream()
.filter(member -> !member.getId().equals(leader.getId())).toList();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The refactored saveTravelItinerary logic has lost several important validations that were present in the original implementation:

  1. UUID Resolution: It does not check if userIdentityResolver.resolve(uuid) returns null. This can lead to null values being passed to the repository.
  2. Member Existence: It no longer verifies that all requested members actually exist. The UserReader.findUsersInIds only checks if the result is not empty, but doesn't ensure the count matches the input.
  3. Group Membership: It no longer verifies that the invited members are actually joined in the group (JoinStatus.JOINED).

These omissions could allow creating travel itineraries with non-existent users or users who are not part of the group.

Comment on lines +77 to 87
public void changeStartAt(final LocalDateTime startAt) {
if (startAt == null) return;
this.startAt = validateStartAt(startAt);
validateDateOrder(this.startAt, this.endAt);
}

private void updateEndAt(LocalDateTime endAt) {
public void changeEndAt(final LocalDateTime endAt) {
if (endAt == null) return;
this.endAt = validateEndAt(endAt);
validateDateOrder(this.startAt, this.endAt);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Moving validateDateOrder into the individual setters changeStartAt and changeEndAt introduces a bug when updating the travel period to a range that does not overlap with the current one. For example, if the current period is [Jan 1, Jan 5] and you try to update it to [Feb 1, Feb 5], calling changeStartAt(Feb 1) will throw an exception because it compares the new start date against the old end date (Jan 5).

In TravelCommand.updateTravel, changeStartAt is called before changeEndAt, making this failure inevitable for any forward shift in the schedule. It is recommended to remove the validation from the setters and either validate once after all changes are applied or provide a dedicated method to update the period (e.g., changePeriod(startAt, endAt)).


public void updateTravel(final Long travelItineraryId, final String title, final LocalDateTime startAt,
final LocalDateTime endAt, final String description) {
TravelItinerary travel = travelReader.findTravel(travelItineraryId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This method performs a redundant database fetch. The caller (TravelUseCase.updateTravel) has already performed authorization checks which likely involved fetching the same entity or its related mappings. Consider passing the TravelItinerary entity directly to this method to avoid unnecessary database round-trips.

Comment on lines +54 to +57
.orElseThrow(() -> new BusinessException(UserTravelItineraryErrorCode.USER_TRAVEL_ITINERARY_NOT_FOUND));
}
return userTravelItineraryJpaRepository.findByUserIdAndTravelItineraryIdAndUserRole(userId, travelId, userRole)
.orElseThrow(() -> new BusinessException(TravelItineraryErrorCode.USER_TRAVEL_NOT_FOUND));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Inconsistent error codes are used for the same logical failure (user not found in travel). Line 54 throws UserTravelItineraryErrorCode.USER_TRAVEL_ITINERARY_NOT_FOUND while line 57 throws TravelItineraryErrorCode.USER_TRAVEL_NOT_FOUND. It is recommended to use a single, consistent error code for these cases to improve maintainability.

Comment on lines +71 to +74
Long addUserId = resolveMemberUserIdOrThrow(req.userUuid());
User addUser = userReader.findUserById(addUserId);

travelCommand.addUserTravelItinerary(addUser, travelItinerary, UserRole.MEMBER);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The addTravelUser method is missing a check to ensure that the user being added (addUser) is a member of the group associated with the travel itinerary. This could allow unauthorized users to be added to private group travels.

Comment on lines 22 to 26
Optional<TravelItinerary> findByIdAndGroupIdAndIsDeletedFalseForUpdate(Long travelItineraryId, Long groupId);

@Query("""
SELECT t FROM TravelItinerary t
WHERE t.group.id = :groupId
AND t.isDeleted = false
ORDER BY t.id DESC
""")
List<TravelItinerary> findGroupTravelsFirstPage(@Param("groupId") Long groupId, Pageable pageable);

@Query("""
SELECT t FROM TravelItinerary t
WHERE t.group.id = :groupId
AND t.isDeleted = false
AND t.id < :cursor
ORDER BY t.id DESC
""")
List<TravelItinerary> findGroupTravelsNextPage(
@Param("groupId") Long groupId,
@Param("cursor") Long cursor,
Pageable pageable
);

@Lock(LockModeType.PESSIMISTIC_WRITE)
@Query("select t from TravelItinerary t where t.id = :travelId and t.isDeleted = false")
@Query("select t from TravelItinerary t where t.id = :travelId")
Optional<TravelItinerary> findByIdAndIsDeletedFalseForUpdate(Long travelId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The method names findByIdAndGroupIdAndIsDeletedFalseForUpdate and findByIdAndIsDeletedFalseForUpdate are now misleading because the isDeleted field has been removed from the entity and the corresponding @Query no longer filters by it. These methods should be renamed to reflect the current implementation (e.g., findByIdAndGroupIdForUpdate).

Comment on lines +24 to +26
if(users.isEmpty()) {
throw new BusinessException(UserErrorCode.USER_NOT_FOUND);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Checking users.isEmpty() is insufficient when findAllById is used. If some IDs in the input list are valid and others are not, JPA will return only the valid ones, and this check will pass. To ensure all requested users were found, you should compare the size of the result list with the number of unique, non-null IDs provided in the input.

@chungjeongsu chungjeongsu merged commit 0f49605 into develop Apr 24, 2026
1 check passed
@chungjeongsu chungjeongsu deleted the refactor/travel branch April 24, 2026 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants